add ripple loader component from trendkite-ui #329
Conversation
| height: 20px; | ||
| } | ||
|
|
||
| .size--lg { |
There was a problem hiding this comment.
sm, md, lg are the sizes copied over from trendkite-ui
| ); | ||
|
|
||
| const iconProp = { | ||
| ...rest, |
There was a problem hiding this comment.
Need a second opinion on spreading custom props in the nested div rather than the wrapper div.
Attaching it to the nested div allows consumer to pass in custom sizes and have it override the default sm, md, lg, xl options.
An alternative option would be to pass a numeric to the size prop rather than enum strings. Then the custom prop can be attached to the wrapper div. This will break away from the enum string pattern used in trendkite-ui
There was a problem hiding this comment.
I don't think I'd call this an "icon", I might just say "Loader" or something, since our icons are usually svgs that you can change colors on.
If the circular image ends up being its own component, it would probably be called Loader, but if it's nested inside a loader component and it would need a distinct name, maybe image, animation, ripple, or something.
There was a problem hiding this comment.
Updated the loader to render with one div and custom props would get spread on that div.
I kept the css that renders the loader's location in the center of its parent html element.
That way consumers can adjust the loader's horizontal positioning with a parent html element that has a defined width and margin.
They could also override the default horizontally centered loader position via style prop if consumers do not have control over the parent html element.
|
At a high-level, I think the structure of this component in tk-ui isn't super clean, and we should take this opportunity to make a change. The main thing that bothers me is something you already mentioned. It's weird that the loader props have to be spread on a child. One of the principles we've been following is that components should affect layout outside themselves. Technically, the loader doesn't violate that rule, but it's weird that the actual circle animation automatically comes with a full width div and centering behavior. My proposal would be:
Looking at Bootstrap and React Material-UI, it looks like they both have components for just the circular bits. |
| ); | ||
|
|
||
| const iconProp = { | ||
| ...rest, |
There was a problem hiding this comment.
I don't think I'd call this an "icon", I might just say "Loader" or something, since our icons are usually svgs that you can change colors on.
If the circular image ends up being its own component, it would probably be called Loader, but if it's nested inside a loader component and it would need a distinct name, maybe image, animation, ripple, or something.
| } | ||
| } | ||
|
|
||
| .loader { |
There was a problem hiding this comment.
With CSS modules, we usually follow a couple of slightly different naming conventions, since we don't need to worry about style collisions.
- The main class for the outermost DOM element wrapper of the component uses the same PascalCase name as the component itself, i.e.
.Loader - Child elements use camelCase class names, and don't need to be prefixed with the component name, i.e.
.icon(although I also mentioned choosing a different name for this). - Levels and sizes generally don't need to be prefixed (i.e. just
.sm), although I'm not sure if this will scale for components that have lots of levels, sizes, states, etc.
There was a problem hiding this comment.
Super minor / non-blocker: I think .loader should still be .Loader
src/components/Loader/story.tsx
Outdated
| }) | ||
| .add('Overview', () => ( | ||
| <FlexWrapper> | ||
| <Loader size="md" /> |
There was a problem hiding this comment.
One more small thing - for the "Overview" stories, we usually instrument all the documented props with "knobs".
Have a look at the Button story for examples if you want:
https://github.com/cision/rover-ui/blob/master/src/components/Button/story.tsx#L42
| } | ||
| } | ||
|
|
||
| .loader { |
There was a problem hiding this comment.
Super minor / non-blocker: I think .loader should still be .Loader
lihnick
force-pushed
the
EVER-13608-add-ripple-loader-to-rover-ui
branch
from
d73c220 to
9efd607
Compare
No description provided.